External-repo review + Track 1 verification spike (EBU R128) - #33
Conversation
…A incorporation Evaluates the three-track incorporation plan against PURPOSE.md and the current ASA baseline (LUFS path in analyze_core.py, JSON schema in JSON_SCHEMA.md, REST surface in server_*.py). Track 1 (LUFS via openmeters): premise unverified. Recommends a 1-day spike (EBU R128 test set + tolerance check) before any rewrite. Flags GPL-3.0 license hazard with openmeters and routes spectral reassignment through librosa.reassigned_spectrogram instead of porting Rust. Track 2 (schema via Partiels): plan misreads Partiels — its CSV is three flat columns, not a richer schema. Reframes to a tabular CSV view per ASA time-series field, modeled on Partiels' (time, duration, value) pattern. Defers SDIF with the paragraph the plan asked for. Track 3 (API via forever-jukebox): side-by-side comparison shows ASA already leads on error envelope, upload size, and staged execution. Adopt list: URL ingestion, public state-machine collapse, audio re-serve, admin key. Reject: Echo Nest output schema, search/discovery, YouTube ingestion. Sequencing matches the plan (2 -> 3 -> 1) with reduced scope: ~6-8 working days total vs the plan's implied effort. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Docs-only PR adding a 306-line evaluation of four external repos against ASA's mission. No code changes. The architectural reasoning is sound and consistent with PURPOSE.md — particularly the rejection of making ASA's JSON "a subset of Partiels" (which would break chain-of-custody invariant #2), the GPL contamination warnings, and the "verify before rewriting" gate on Track 1. Sequencing recommendation is correct.
Findings
Should fix:
docs/external-repo-review-2026-05-13.md, Track 3 side-by-side table — the document says "100 MiB raw / 105 MiB request envelope" for ASA's upload limit. The actual envelope is 101 MiB: MAX_UPLOAD_SIZE_BYTES = 104_857_600 (100 MiB) + UPLOAD_REQUEST_SIZE_SLACK_BYTES = 1_048_576 (1 MiB) = 105,906,176 bytes = 101 MiB. The PR description itself has this right (says 101 MiB). Worth fixing before the doc gets referenced as the canonical source.
Worth considering:
The CSV exporter section calls the proposed …/export/csv/{field_path} route a "JSONPath syntax" path (e.g. lufsCurve.shortTerm). What's described is a simple dot-path, not JSONPath (which uses $. prefix and [*] for arrays). Call it "dot-path" or "field path" to avoid confusion when someone later implements it and reaches for a JSONPath library expecting it to work.
Test results
No tests to run — docs-only PR. No executable code changed.
Phase boundary check
Clean. No code changes; no phase boundary to violate.
Generated by Claude Code
Two corrections to docs/external-repo-review-2026-05-13.md per PR #33 review: 1. Track 3 baseline said '105 MiB request envelope'. Correct value is 101 MiB: MAX_UPLOAD_SIZE_BYTES (104,857,600) + UPLOAD_REQUEST_SIZE_SLACK_BYTES (1,048,576) = 105,906,176 bytes = 101 MiB exactly. The PR description already had this right; the doc body did not. Added the arithmetic inline so the next person doesn't have to redo it. 2. Risks table called the CSV exporter route a 'JSONPath' resolver. What's described is a simple dot-path (e.g. 'lufsCurve.shortTerm'), not JSONPath syntax (which uses '$.' and '[*]'). Clarified to avoid anyone reaching for a JSONPath library when implementing. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
… 1 & 2 Adds tests/test_loudness_r128.py that exercises ASA's analyze_loudness path against the EBU R128 Tech 3341 compliance signals: - Case 1: stereo 1 kHz sine at -23 dB FS, 20 s @ 44.1 kHz → expected integrated loudness -23.0 ±0.1 LUFS. - Case 2: stereo 1 kHz sine at -33 dB FS, 20 s @ 44.1 kHz → expected -33.0 ±0.1 LUFS. - Case 1 at 48 kHz with explicit sampleRate=48000 (bypasses analyze_loudness): proves the algorithm itself is sample-rate aware when Essentia is told the right rate. Signals are generated procedurally — no fixture downloads, no audio files committed. This satisfies the review's recommendation in docs/external-repo-review-2026-05-13.md to verify ASA's loudness path against EBU R128 before any port of openmeters' BS.1770-5 implementation. If both 44.1 kHz tests pass on CI, the openmeters port stays rejected and this becomes the regression gate. Open finding surfaced during the spike — filed as a follow-up, NOT fixed here: - analyze_core.analyze_loudness instantiates LoudnessEBUR128() with no sampleRate (defaults to 44100), but load_stereo returns audio at the source file's native rate without resampling. analyze_fast.py already does the right thing (sampleRate=sample_rate). Documented in docs/track1-spike-outcome-2026-05-13.md with a concrete fix sketch and the affected call sites. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Closes the open finding from PR #33's Track 1 verification spike. Before this PR, analyze_core.analyze_loudness instantiated es.LoudnessEBUR128() with no sampleRate argument (Essentia default 44100), but the full pipeline at analyze.py:1497 passed in a stereo array from load_stereo at the source file's native rate (no upstream resample). Any source above 44.1 kHz got K-weighting tuned for the wrong rate. This PR: - analyze_loudness now takes sample_rate: int = 44_100 and threads it to LoudnessEBUR128(sampleRate=sample_rate). - analyze.py:1503 (full pipeline) passes sr from load_stereo through. - analyze.py:1242 (stem path) hardcodes 44_100 because Demucs writes stems at 44.1 kHz regardless of source rate. - New regression test asserts -23.0 ±0.1 LUFS through analyze_loudness at 48 kHz. - Three mock-based wiring tests (per review feedback) assert sampleRate=<value> reaches Essentia, including the verbatim case for 96 kHz. - Docstring on the spike outcome doc marks the finding as resolved. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
…nt (#43) * feat(spectral): librosa.reassigned_spectrogram as an opt-in enhancement Last item from the merged external-repo review's adopt list. Adds a new on-demand spectral-enhancement kind, 'reassigned', that produces a sharper spectrogram via per-bin (time, frequency) reassignment. Why reassignment: vanilla STFT spectrograms smear transients across the analyzing window (~46 ms for n_fft=2048 at 44.1 kHz). Reassignment relocates each STFT bin to the local centroid of energy in the (time, freq) plane — transients land at their true onset and stable partials collapse to sharp horizontal lines. Useful when a producer is trying to read attack timing or harmonic detail from the existing mel/STFT views and finding them too blurry. Implementation: - apps/backend/spectral_viz.py — new generate_reassigned_spectrogram function. Calls librosa.reassigned_spectrogram with both frequency and time reassignment enabled, fill_nan=True (handles silent-region numerical instability without dropping points), and renders via matplotlib scatter (not librosa.display.specshow — specshow would re-rasterize to a uniform grid and undo the sharpening). Magnitude floor of -60 dBFS below peak filters the noise wash without losing attack detail. - apps/backend/server.py — registered as 'reassigned' in the existing _ENHANCEMENT_GENERATORS dict. Inherits all existing wiring: auth, measurement-completed precondition, idempotency, artifact storage, MIME types, JSON response envelope. - apps/backend/tests/test_spectral_viz.py — 3 new tests: - produces_single_reassigned_png: shape + non-trivial size - output_is_valid_png: magic bytes - handles_silent_input_without_raising: regression gate against NaN propagation on near-silent fixtures No new dependencies — librosa is already in requirements.txt (librosa==0.11.0 exposes reassigned_spectrogram). Usage: POST /api/analysis-runs/{run_id}/spectral-enhancements/reassigned Returns the new 'spectrogram_reassigned' artifact ID alongside the existing PNG-style spectrograms. Idempotent: re-posting returns the existing artifact without regenerating. Out of scope (deferred): - Frontend UI toggle to display the reassigned PNG. The backend now produces it on demand; the UI wiring is a separate follow-up. - Raw (time, freq, mag) JSON artifact for client-side custom visualization. Could be added as a sibling 'reassigned_data' artifact if a future consumer needs it. Closes the last review-adopt item. Full session summary in the merge commits for #33 through #42. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg * perf(spectral): cap reassigned-scatter at 300K points PR #43 review caught a real perf issue: the original generate_reassigned_spectrogram fed every STFT cell into matplotlib.scatter unchecked. At default params (n_fft=2048, hop=1024, sr=44100), that's: ~8M raw points on a 3-minute track ~15M raw points on a 10-minute track Both well above the floor-mask survival count and well above the threshold where matplotlib's render step starts taking tens of seconds. The librosa compute cost isn't the bottleneck — the scatter render is. Fix: - New REASSIGNED_MAX_SCATTER_POINTS = 300_000 constant. At FIG_DPI=100 / ~1200x400 px figure, 300K is the visual sweet spot — sharper-looking than vanilla STFT, renders in under a second. - After the floor mask, the post-mask arrays (times_visible, freqs_visible, mags_visible) get subsampled via a seeded RNG (np.random.default_rng(0)). Seeded so re-running on the same audio produces a byte-identical PNG — matters for idempotency tests and content-hash artifact caching. - New test test_scatter_point_cap_is_enforced_for_long_inputs: generates a 60 s multi-component fixture (~1M+ post-mask points, comfortably above the 300K cap), patches matplotlib.scatter to capture call args without rendering, and asserts the actual point count passed to scatter is <= REASSIGNED_MAX_SCATTER_POINTS. The 'inferno colormap' worth-noting from the review was checked — no 'inferno' reference exists in CLAUDE.md or anywhere in the codebase. The reviewer's worth-noting was itself stale; nothing to fix there. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg --------- Co-authored-by: Claude <noreply@anthropic.com>
Two layers of work on the external-repo incorporation plan:
docs/external-repo-review-2026-05-13.md) — evaluates openmeters, soundscope, Partiels, and forever-jukebox against PURPOSE.md and ASA's current implementation. Adopt/adapt/reject per track with reasoning.TL;DR — the review
LoudnessEBUR128is likely close enough on stereo material that 1770-5 deltas fall below ASA's reported precision (±0.1 LU). Wrong runtime target (Rust → Python, not Rust → JS — there is no Essentia.js here).time, duration, label/value). ASA's schema is richer, not poorer.X-Admin-Key. Reject: Echo Nest schema, Spotify/YouTube search.Track 1 verification spike — added in commit
8c7da4capps/backend/tests/test_loudness_r128.py— three procedurally-generated tests, no fixture downloads:test_case1_minus23_dbfs_sine_yields_minus23_lufsanalyze_loudness(stereo)test_case2_minus33_dbfs_sine_yields_minus33_lufsanalyze_loudness(stereo)test_case1_at_48khz_with_explicit_sample_ratees.LoudnessEBUR128(sampleRate=48000)directlyThe third probe separates "algorithm correct at non-44.1 rates" from "ASA's call site threads the sample rate" — the difference matters for the open finding below.
docs/track1-spike-outcome-2026-05-13.md— the outcome doc that this spike closes out against. If CI is green, Track 1's openmeters port stays rejected.Open finding: sample-rate threading bug in
analyze_loudnessSurfaced while tracing the call sites for the spike. Not fixed in this PR (out of scope for a verification-only spike), but filed:
analyze_core.py:197—es.LoudnessEBUR128()instantiated with nosampleRate(defaults to 44100).analyze_fast.py:100—es.LoudnessEBUR128(sampleRate=sample_rate)— does the right thing.analyze.py:1448—load_stereoreturns audio at the source's native rate without resampling.So a 48 kHz FLAC reference track today gets its loudness measured against K-weighting filters tuned for 44.1 kHz. The integrated-LUFS error is small at 1 kHz but grows with frequency. Fix sketch (in the outcome doc): thread
sample_ratethroughanalyze_loudness, update call sites atanalyze.py:1242andanalyze.py:1497. Small follow-up PR.What this PR does NOT do
librosa.reassigned_spectrogram— separate small add per the review.Open questions for the human
POST /api/analysis-runsURL-mode lands, do we want host-allowlisting in hosted mode by default?…/export/csv/{field_path}using a simple dot-path (lufsCurve.shortTerm). Open to typed routes per series if preferred.https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg